-
Notifications
You must be signed in to change notification settings - Fork 747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: added support to remote files #141
Conversation
- Sound.js now checks if is RelativePath using also http and https - RNSoundModule now uses setAudioStreamType(AudioManager.STREAM_MUSIC) to stream music - Exception now are handled differently
mediaPlayer.setDataSource(fileName); | ||
} catch(IOException e) { | ||
Log.e("RNSoundModule", "Exception", e); | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice - streaming is a good idea over http for sure.
mediaPlayer.setDataSource(fileName); | ||
} catch(IOException e) { | ||
Log.e("RNSoundModule", "Exception", e); | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have a specific reason not to invoke a callback error when the setDataSource throws an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it usually throws an exception when internet is missing, or when the file does not exist. Do you suggest to handle differently those two? (returning null here, will invoke the error callback anyway later on with a more generic "Resouce not found" error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheGhoul21 ah - thanks, I didn't notice that it'd hit the error callback anyway. Thanks for the clarification. This seems a sensible solution
Any chance this gets merged soon ? |
@matthieugayon Let me know if it works for you, and if so we should merge it. |
Ok I'll try to get back to you asap on that, most probably tomorrow |
EDIT: Ignore me! I should have set up CodePush before commenting. I just set it up, and was surprised to discover that it can already manage my sound files out of the box, thanks to React Native's asset manager. It's pretty incredible that I can just push out new sounds to my users. |
+1 for this getting merged, it's the missing link for react native audio playing |
This is actually great timing. As I mentioned, I've been setting up CodePush, and switching to React Native's asset manager. So I just realized that I actually do need this, because now I get a URL during development: It also might be good to add "Load sound from React Native Static Resources" to the feature matrix. I didn't realize it was possible to play sounds this way, until I stumbled onto that page. |
I just replaced react-native-sound with TheGhoul21/react-native-sound in my cross platform app and can confirm that that Android load from network works identical to iOS now. It should probably be in the readme somewhere that the syntax to load from network is |
@SteffeyDev Thanks for the confirmation. I've merged it now. Great job @TheGhoul21 |
* feat: added support to remote files - Sound.js now checks if is RelativePath using also http and https - RNSoundModule now uses setAudioStreamType(AudioManager.STREAM_MUSIC) to stream music - Exception now are handled differently * fix: amended README specifying support for android network streaming
to stream music